fix: gate tool execution on raw stream completion, not just JSON validity - #3434

Thanks for moving the tool-execution decision onto raw stream evidence and for adding a broad settlement matrix. The final safety boundary still has two fail-open/production-readiness issues and one duplicated payload authority. I left the required final state inline; the core simplification is that every streamed call must execute only the identity and value that the guard actually proved.\n\nAI-assisted review disclosure: Codex delegated independent Runtime and test reviews; I verified the exact-head control flow, dependency declaration, and live PR state before posting.

Thanks — the defect you found is real and it is the good kind of finding: settleModelStepOutcome classifying finishReason: "length" as completed is correct for its own purpose (continuation and retry bookkeeping) and wrong as an execution gate, and nothing in the type system was ever going to tell anyone that those two questions had been conflated. A mutating call reaching ToolRuntime because its arguments happened to be syntactically complete, while the generation that produced it was cut off mid-thought, is exactly the failure worth closing.

The second half of the argument is the stronger one, and I want to say so explicitly because it is easy to miss: tool-call.input is post-processed data that repairToolCall may have coerced, whereas the raw tool-input-delta bytes are evidence about what the provider actually streamed. Verifying against the bytes rather than the SDK's conclusion is the right authority.

The handling of atomic delivery is the part I went in expecting to find a hole in and did not. Treating "no raw evidence for this id" as safe-by-omission would have been the obvious mistake; instead the absence is only allowed to mean "genuinely atomic" when hadRawArgumentEvidence is false for the whole request, because once any call in the request streamed real bytes, another call's missing decision is indistinguishable from an id mismatch. That distinction is subtle and it is drawn correctly.

The tests are proportionate to what they protect. 25 behavioural cases asserting execution counts — zero times / exactly once — across a safety matrix and a red-team set, including abort mid-stream, a missing terminal event, and two concurrent runs sharing toolCallId: "call_1". These fail if the gate regresses, rather than restating it.

So: no P0, and nothing wrong with the mechanism.

My one blocking concern is not about the code at all — it is about the dependency, and I do not think it can be settled inside this PR.

This adds prefix-safe-json@0.1.1 and makes it the authority that decides whether tool calls with real side effects — filesystem writes, shell commands, apply_patch, SQL, dependency installs, by your own list — are allowed to execute. Per THIRD_PARTY_NOTICES.txt, that package's repository is github.com/canblmz1/prefix-safe-json, which is your own account.

I want to be clear about what I am and am not saying. I am not suggesting anything improper, the license is clean (MIT OR Apache-2.0, Apache-2.0 selected), pinning the exact version rather than a range is the right call, and writing the hard part as a reusable library is a defensible engineering decision. What I am saying is that an Apache project taking a security-critical runtime dependency on a pre-1.0 package owned by an individual outside the project's control is a decision the project has to make deliberately, and right now it is a single line in a bugfix PR whose description does not mention it. Someone reviewing the Problem/Fix sections would not learn that this happened.

The questions I would want answered before this lands, none of which I can answer for you:

• What happens to this gate if the package is unmaintained, unpublished, or its npm publish rights are compromised? The blast radius is "tool calls execute when they should not", which is the thing this PR exists to prevent.
• Is donating the code to the project — vendoring it under packages/, with the same tests — on the table? The consumed surface here is one factory and its verdicts; the completeness parser is the substance. That would keep the design and remove the external ownership question entirely.
• Has an ASF-side dependency review been done? I do not know this project's threshold for new runtime dependencies, and that genuinely is a question for a maintainer rather than for me.

If the answer is "vendor it", nothing about your design has to change, which is why I think this is worth raising rather than working around.

On CI: test and package are red on this head, and neither is your fault. Both fail on pi-tui-runner.ts missing midTurn, a file this PR does not touch. This run was created at 11:41:11Z; the commit that added midTurn: 'local' landed on main at 11:49:56Z. A pull_request run tests the merge commit computed at event time, so this one predates the fix, and a re-run replays the same SHA. A rebase onto current main should clear it — please do not go looking for something to fix there.

Review assisted by AI (Claude Opus 5). Findings were verified against the files, the lockfile, the third-party notices and the workflow-run timestamps at this head; the reviewer is accountable for them.

Thanks — the underlying problem is real and worth fixing. settleModelStepOutcome classifying finishReason: "length" as { kind: 'completed' } is correct for continuation bookkeeping and wrong as an irreversible execution gate, and separating those two questions instead of changing the shared classifier is the right instinct. The length-cutoff-tool-execution-repro test is a genuine reproduction, not a synthetic one.

Reviewed at exact head 39214a7cb0a9153b08d24c880ff23a220dd63695 against base 4acfa26934ce4b2b385b76f8a11048bb83fab861. One P1 and two P2, inline. Plus one question we could not settle, and two verification gaps.

This branch has no CI evidence at all. gh pr checks reports no checks on this head — the description says all three workflows are action_required pending maintainer approval. So the 870 lines of new tests have never run anywhere except locally. We ran @maka/runtime in full ourselves: 3120 tests, 3093 pass, 14 fail, 13 skip. All three of the PR's own suites pass (tool execution safety (real production path) 22, tool-call-execution-guard 29, isSafeToolExecutionStepOutcome).

About those 14 failures — they are not yours, and the cause is worth recording. They come from installing with npm ci --ignore-scripts, which skips patch-package. This repo patches @ai-sdk/provider-utils, and that patch rewrites StreamingToolCallTracker wholesale — replacing toolCallsById/toolCallsByIndex with a single array and adding absentIfBlank(). Without the patch, model-factory-tool-call-index.test.ts fails 9 of its 11 cases. We confirmed by running that file on both main and this head on a patched checkout: 11/11 green on both. The remaining 5 are macOS-vs-Linux platform differences. Anyone reproducing this PR locally should install without --ignore-scripts.

The description says targeted runtime tests 257/257 passing; we could not reproduce that number. We can confirm the 51 tests in the two new files plus isSafeToolExecutionStepOutcome all pass. Given F1 below, we would not treat the 257 as established.

A structural suggestion. This PR contains two separable changes: (a) refusing to execute a tool call whose stream was cut off by a token limit, which is well-evidenced and lands cleanly; and (b) the atomic-delivery fallback that F2 and F3 are about, whose stated justification does not hold up. Splitting (a) into its own PR would let the good half merge now while (b) gets the threat model it needs.

This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

Thanks — and this one is us correcting ourselves, not asking anything new of you.

Reviewed at exact head 39214a7cb0a9153b08d24c880ff23a220dd63695 against base 4acfa26934ce4b2b385b76f8a11048bb83fab861. No checks have run on this head yet, so nothing here is CI-backed.

Our P1 on this head was framed wrongly, and the framing was unfair to you.

We reported that the description's Dependency section promises prefix-safe-json@0.1.1 and a set of license/notice changes that do not exist in the diff. That is factually true of this head — but we presented it as a description that overstates the work, and that is not what happened.

What actually happened is this. Our earlier review at head 452ff41fd raised the dependency as the one blocking concern and asked:

Is donating the code to the project — vendoring it under packages/, with the same tests — on the table? […] If the answer is "vendor it", nothing about your design has to change.

At 452ff41fd the dependency was genuinely there: packages/runtime/package.json:148 carried "prefix-safe-json": "0.1.1", and the diff included package-lock.json, three THIRD_PARTY_NOTICES.txt files and scripts/generate-third-party-notices.mjs. The description was accurate when it was written.

You then force-pushed 39214a7cb, which removes the dependency and inlines the completeness check into tool-call-execution-guard.ts using a plain JSON.parse. Those five dependency and license files drop out of the diff, leaving the seven runtime source files. You did the thing we asked for, and you did it without arguing about it. The only thing left behind is a description that still describes the previous approach.

So the substance of that item is unchanged but its nature is not: please update the PR description to match this head. That is housekeeping on a change that already went the right way — not a discrepancy to answer for.

Why we missed it: we locked head, base, mergeability and CI for this head, but did not check whether the existing reviews on this PR were bound to it. Seven reviews already existed; four of them sit on 452ff41fd. Had we looked, the first hypothesis for "description does not match implementation" would have been "the head moved", which is exactly what it was. We have made "are the existing reviews bound to the current head?" a required part of our own pre-flight so this does not recur.

Unchanged from the previous review — these were about this head's code and still stand:

• the comment in tool-call-execution-guard.ts citing computer-use-provider-protocol.test.ts as verification for zero-delta atomic delivery, where that file contains no tool-input-* chunks at all;
• the mixed-delivery consequence in ai-sdk-backend.ts that follows from it;
• the open question about provedValue bypassing the SDK's schema-validated projection, which we flagged as a question precisely because we had not executed it.

Sorry for the mischaracterisation. The gate itself is a good change.

This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

Thanks, and apologies — this is our second correction in a row on this PR, and this one is larger than the last. Three of the four items we raised at this head should not have been raised. The short version: you answered all three of our earlier findings, in writing, on 22 Aug at 11:42–11:43, and we did not read your replies before reviewing again.

Reviewed at exact head 39214a7cb0a9153b08d24c880ff23a220dd63695 against base 4acfa26934ce4b2b385b76f8a11048bb83fab861. No checks have run on this head. Nothing below asks you to change code.

Confirming your three Fixed. replies, each against the code at this head.

1. Request-scoped atomic fallback — correct, and it is what we asked for. Our 09:23 review said: "Please make 'no raw evidence anywhere in this request' the only case eligible for the atomic fallback; otherwise require every final call ID/identity to have an explicit execute verdict. Add mismatched-ID and mixed atomic/incremental regression cases." You implemented exactly that, and the regression cases exist by name — an atomic sibling is blocked once the same request contains any raw argument evidence, concurrent incremental requests sharing call_1 stay isolated, concurrent atomic requests sharing call_1 stay isolated.

We then filed that same behaviour back at you as a [P2] ("Mixed delivery in one request rejects a legitimate tool call"). That finding is withdrawn. Request-global hadRawArgumentEvidence tightens the gate: it is the only condition under which the atomic fallback is permitted at all, and once any call in the request has streamed real bytes, a call missing its own decision fails closed instead of borrowing the step outcome. We read a fail-closed hardening as a fail-open scope bug.

Worse, we proposed "scoping the evidence per call rather than per request" as a smaller fix. Please disregard that suggestion. Per-call scoping is precisely the call_1/call_2 id-mismatch gap our own earlier [P1] asked you to close. Acting on it would have reopened the hole.

2. Proved value as the sole payload authority — correct, and also what we asked for. Our 09:23 [P2] said: "Please carry the guard's proved identity/value into the execution decision and make it the sole payload authority… A regression test should assert the exact object delivered to ToolRuntime, not only the boolean verdict." You did, and ToolRuntime receives the raw-proved object, never divergent projected input asserts it.

Our [Question] about provedValue versus the SDK's schema-validated projection was written as though the switch were your design choice. It was our request. The observation itself may still be worth someone's attention — if the AI SDK writes Zod defaults into toolCall.input, then tools like archive-read-tool.ts:41 (operation: …default('inspect')) would see undefined where they previously saw 'inspect', since validateDeclaredToolArgs validates and discards. But that is a consequence of the design we asked for, and we still have not reproduced it. It is a note on our own request, not a question for you to answer.

3. Dependency — already covered in our previous comment. Withdrawn there; it was removed at our request and only the description lagged.

The one item that survives, downgraded. Our [P2] about the comment in tool-call-execution-guard.ts citing computer-use-provider-protocol.test.ts is literally true — that file is 1269 lines and contains no tool-input-* chunks. But we wrote "we could not find evidence that does", and that was us not looking. This PR's own tests observe those chunks 14 times in tool-call-execution-guard.test.ts and 7 times each in length-cutoff-tool-execution-repro.test.ts and ai-sdk-backend.test.ts. So this is [P3]: the comment names the wrong file. Not a coverage gap. Point it at a test that actually observes the chunk sequence and it is done.

Root cause, so you can hold us to it. We locked head, base, mergeability and CI, and never fetched the PR's review comments. Your replies were addressed to us and sat unread while we re-reported the things they resolved. "Read the author's point-by-point response to the previous round" is now a required step in our pre-flight, alongside "are the existing reviews bound to the current head".

Net state at this head: no P0, no P1, no P2 from us. One P3 (wrong file named in a comment) and one stale description section. The gate is a good change and it got better under review.

This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

Quick note on the CI failure at e106669d5, since it's a cheap one.

test is red only on Check formatting — that's the sole failing step; nothing in the actual test run fails. Biome reports two errors, both in files this PR adds:

• packages/runtime/src/__tests__/length-cutoff-tool-execution-repro.test.ts
• packages/runtime/src/__tests__/tool-call-execution-guard.test.ts

It's the multi-line object formatting in the satisfies ModelStepOutcome[] array. npm run format should clear it.

I haven't reviewed this head yet — I'll do that once CI is green. Flagging it now so you're not waiting on me to find out it's a formatting fix.

This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are mine to correct — please push back where I got it wrong.

Reviewed at head 9c92f47e. One P2, no P0/P1.

On the premise first, because it's the part that makes this worth the size: complete JSON is not evidence that the step around it ended safely, and settleModelStepOutcome treating length as completed meant a tool call could reach ToolRuntime after generation was cut off at the token limit. Failing closed on irreversible side effects is the right default, and splitting the raw-bytes layer from the step layer is what makes it enforceable — tightening length alone wouldn't catch "raw bytes truncated, SDK repaired them, executed anyway." Per-physical-request trackers and preferring proven name/value over the SDK projection both follow from that.

A note on CI freshness: test is SUCCESS but started 2026-08-22T18:27:44Z against base 4acfa2693, and main has since moved to 3ab0605b. For what it's worth, ai-sdk-backend.ts, model-adapter.ts, and model-protocol.ts haven't changed on main since that base, so the drift is empty for the files this touches.

I did not re-raise the earlier P1s (alpha dependency, description/branch mismatch) — they're gone from this head — nor mixed incremental+atomic failing closed for the whole request, which reads as deliberate.

[P2] "The stream finished" now has two authorities that disagree, and the guard has the better answer available but discards it.

model-adapter.ts:746 resolves a finish reason through chunkFinishReason, which deliberately falls back to the provider's raw spelling when unified is other or unknown. Its comment states the rule plainly: other with a provider spelling is an ordinary finished turn; other with nothing behind it is a stream that died silently.

tool-call-execution-guard.ts:201 uses a second resolver, normalizedFinishReason at :122, which reads .unified and stops there. So for a finish chunk of { unified: 'other', raw: 'stop' }:

• step settlement sees a completed step with stop, and an atomic tool call executes;
• the tracker sees unified other, marks terminal unsafe, and every call with raw deltas is rejected.

Same physical request, opposite outcomes, and the deciding factor is which of two functions asked the question — not whether the stream actually ended.

What makes this cheap to fix is that the information is already at the boundary. model-adapter.ts:438 calls resolveToolCallSafety(toolCallGuard, { providerReason: finishReason }) with the raw-aware value, and resolveToolCallSafety at :227 names the parameter _meta and never reads it.

I want to be careful about the docstring at :224, since it looks like this was considered: "meta is accepted for the ModelAdapter call shape but cannot promote a stream with no terminal event to safe." That concern is legitimate and worth keeping — meta must not rescue a stream that never produced a terminal event. But the divergence above is not that case. There was a terminal finish chunk; it was classified by the weaker of two resolvers. Consulting providerReason only when tracker.terminal was set by an actual finish chunk honours the stated intent and still closes the gap. Routing the guard's finish classification through chunkFinishReason would work equally well if the raw spelling is reachable at that layer.

One more thing I looked at and am not grading: JSON.parse(state.raw) accepts non-objects, so null or true parses to a "proven value." Whether that can reach a tool depends on downstream schema validation, which I didn't chase far enough to claim either way.

Two independent lines on this one (different models), plus a check of the delta between the head each was bound to.

The earlier finding is fixed. Our first line, reviewing 9c92f47e, reported a real problem: resolveToolCallSafety took its second parameter as _meta — accepted and then discarded. ModelAdapter was already passing { providerReason: finishReason } at that head, so the value was computed and thrown away. The consequence was two authorities on "the request ended": step settlement used chunkFinishReason, which falls back to the provider's own spelling when the SDK's unified reason is ambiguous, while the tracker only ever read .unified. On a { unified: 'other', raw: 'stop' } finish that split — the step settled as completed, so an atomic call could execute, while every call with raw deltas was refused. Same physical request, opposite verdicts, and the reason was which of the two finish functions you asked.

On 80dacb4b the parameter is meta and isTerminalSafe actually consumes it, with the precedence written down: providerReason can only resolve an ambiguous local classification (undefined/other/unknown), and can never lift length, content-filter, or an explicit error/abort into safe. We diffed the two heads — the change is confined to tool-call-execution-guard.ts and its two test files, with 74 new lines of guard tests covering exactly this case. The two authorities now agree by construction rather than by coincidence.

Second line, on the current head, found nothing at P0–P3. Directions it checked and ruled out, with reasons:

• Mixed evidence in one request (one streamed call, one atomic sibling) refuses the atomic one. This is deliberate fail-closed behaviour with a dedicated test, not a defect.
• The INVALID_TOOL_NAME path: decision = execute with a mismatched name yields dispatched: 'invalid'. Traced into repairMakaToolCall — that path only renames unrepairable calls and synthesizes a { tool, error } input; the invalid tool implementation formats an error and performs no side effect, and provedValue is undefined so the fallback correctly describes the original call.
• step-finish is translated as its own event, and sawFinish only accepts kind === 'finish', so guard and settlement do not fork.
• Cross-request id collision: a tracker is instantiated per startStream, with concurrent same-id isolation covered in tests.
• Exception paths: resolveToolCallSafety sits in an inner finally, so a failed await sdk.response still settles.

One design note, not a blocker. This does introduce a second authority — raw bytes outrank the SDK's projection for both the tool name and the payload. That is the right call here: narrowing the existing gate with finishReason !== 'length' would close the truncation hole but not the two others this covers (post-hoc repair/coercion replacing toolCall.input, and identity substitution). But the precedence rule currently lives in comments and type docs. It is worth keeping an eye on: the next person to touch either side needs to know which one wins, and a comment is a weaker guarantee than a type or a test. Not something to hold this PR for.

On entropy, honestly: this one adds. +1444/-4, of which roughly 969 lines are tests and ~470 production, and nothing old is removed — the new evidence channel runs alongside the existing gate rather than replacing it. What earns it that cost is that it stops completed from being the execution gate at all, and covers truncation, identity substitution and payload substitution through one mechanism instead of three patches.

CI is terminal green on 80dacb4bcdd922a08b15dc1a656400db2bca91a2 (test: completed / success), which is the head both this approval and the second review line are bound to.

Approving.

A maintainer asked for this PR to be re-reviewed from scratch, on the grounds that it went back and forth a lot — seven reviews across five heads before the approval. That is a fair concern: a final approval on a long thread is easy to reach by "everything I complained about looks addressed" rather than by re-deriving the conclusion. So this was re-derived independently at exact head 80dacb4bcdd922a08b15dc1a656400db2bca91a2, deliberately before reading any of the prior reviews.

The approval stands. No new findings. What follows is the evidence that was missing from the record, not a restatement of it.

The two authorities are genuinely unified, not coincidentally agreeing. The guard reads only the finish event's .unified; providerReason may rescue it only when that lands on other/unknown/undefined. And the string it rescues with is the same one step settlement uses — model-adapter.ts:439 passes the finishReason resolved as streamedFinishReason ?? rawFinishReasonString(sdk.finishReason) ?? 'unknown', which is the same value fed to settleModelStepOutcome. The two now share an input rather than computing the answer twice. isTerminalSafe cannot promote pending or blocked, and cannot override length or content-filter — only an ambiguous classification. The doc comment above it states each of those limits explicitly, including why the guard cannot simply import chunkFinishReason (model-adapter.ts imports the guard, so the reverse would cycle).

There is no third judge of "the request ended", and the most suspicious exit was checked specifically. One physical sdk.stream can carry several steps, each closed by finish-step with a terminal finish at the end. If the tracker treated finish-step as terminal, the second step's tool evidence would poison the whole request. It does not: finish-step has no case in observeRawChunk's switch and falls to default: return. Only a terminal finish sets tracker.terminal — and a second finish-shaped event is treated as blocked rather than being allowed to silently replace the first, which is the right call and is commented as such. The reverse divergence — settlement failing while the guard permits — is unreachable: ai-sdk-backend.ts:2690 throws on providerOutcome.kind !== 'completed' before any of this is consulted.

The new guard does not relocate the old problem. Every mismatch exit fails closed: ambiguous with no providerReason, length, pending, truncated JSON, a missing id while a sibling streamed deltas, a toolName conflict, tool evidence after the terminal event. An adversarial probe matrix over these was run against the built modules at this head, and each landed on reject.

The previously reported resolveToolCallSafety finding is closed. The signature is now (tracker, meta?: { providerReason?: string }) and meta.providerReason is consumed in exactly one place — the ambiguous branch of isTerminalSafe. No remaining parameter is accepted and discarded.

Existing suites at this head: guard plus repro 60/60, ai-sdk-backend 204/204. Required test is completed / success bound to this exact SHA.

Disclosure: the reviewer who re-derived this had previously run one of the earlier lines on this PR, so this is a fresh derivation rather than an uncontaminated first look. The derivation was completed before any prior review was read, and the comparison against them was done only afterwards.

AI-assisted review. The unification of the two authorities, the finish-step handling, the settlement-side throw, and the closure of the earlier finding were each verified against the source at this exact head. Under CONTRIBUTING.md §Review this does not count as the required independent human review.

Follow-up simplify audit at exact head 80dacb4bcdd922a08b15dc1a656400db2bca91a2, prompted by a maintainer question: this closes a small safety boundary, so why is the change this large?

The answer is that the capability is justified — but the PR is carrying two authorities where one would do. The security capability should stay: Maka does need a tool-execution gate that rejects length and, where providers expose incremental argument bytes, executes only the raw-proved tool identity and value. ToolRuntime is not a duplicate of that — its schema/admission/permission checks are orthogonal and also serve Code Mode and nested callers that never pass through the provider stream. Changing settleModelStepOutcome so that length classifies as a failure is also not the smaller fix: it would turn ordinary max_tokens completion into provider-error/retry semantics.

Two reductions are worth making before merge.

[P1] Let ModelStepOutcome be the only terminal-reason authority

The guard currently does two different jobs: raw argument proof, and a second terminal-reason verdict.

• packages/runtime/src/model-adapter.ts:405-415 derives finishReason and settles ModelStepOutcome.
• packages/runtime/src/tool-call-execution-guard.ts:136-146 separately normalizes the raw finish reason.
• packages/runtime/src/tool-call-execution-guard.ts:235-261 separately decides whether that reason is execution-safe, and reconciles ambiguity against providerReason.
• packages/runtime/src/tool-call-execution-guard.ts:316-320 already holds the canonical predicate over ModelStepOutcome.

That second job already drifted once inside this PR (unified: other against a provider raw stop) and needed a follow-up commit to reconcile the two views — which is the usual sign of duplicated authority rather than defence in depth.

Suggested shape: keep terminal state as pending | finish-observed | blocked only, resolve raw proofs against the already-settled ModelStepOutcome from the same ModelAdapter finally block, and authorize a raw proof only when the state is finish-observed (not blocked) and isSafeToolExecutionStepOutcome(settled) holds. normalizedFinishReason, the local terminal reason, isTerminalSafe, and the providerReason reconciliation contract then all go away. This is a closed replacement: missing finish and poisoned ordering stay tracker facts, while stop/tool-calls versus length/failure stays the settled outcome's fact, so every current adversarial case maps to the same result. Roughly 35–55 production lines and 40–70 test/comment lines, plus one whole concept.

If maintainers deliberately want a cross-check against ModelAdapter contradicting its own raw stream, that is a legitimate choice — but then please record it as an intentional second authority rather than leaving it as incidental duplication.

[P1] Represent only positive proofs

ToolCallSafetyDecision.action is declared execute | retry | reject, but production constructs only execute and reject — retry has no producer and no consumer anywhere in the repository. Beyond that, in any request that has raw evidence, a rejected decision and an absent proof reach the same backend result: fail closed through the invalid-tool result path.

Replacing decisions with a map of positive proofs only ({ name, value }) and keeping hadRawArgumentEvidence for the all-atomic fallback removes an impossible state and a distinction nothing reads. Invalid, incomplete, malformed, or terminal-unsafe raw calls simply have no proof. A deletion probe narrowing the type to action: 'execute' and omitting rejected entries was clean under git diff --check and Biome, at a net 5 lines across the two production files; with the type/docs/backend branch cleanup, about 8–18 lines.

[P2] Follow-ups, fine to defer

• The long-form contract explanation is repeated in four places; one owner in the guard would do. For scale: of 479 added production lines, 237 are comment/doc.
• Reusing the existing durable backend fixture instead of new scaffolding looks worth about 90–120 test lines.

[P3] Outside this PR

The backend's isIncompleteProviderFinishReason and settleModelStepOutcome carry separate incomplete-finish predicates; sharing one is a separate cleanup, not this PR's job.

On the earlier approval on this head: it was published before this audit and should not be read as clearing the two [P1] items above. Once they are addressed — or the cross-check is explicitly recorded as an intended second authority — this is good to go from our side.

Correcting our own earlier review on this PR.

In review 5002108780 we raised a [P1] recommending that the guard stop classifying the terminal finish reason itself and instead consume the already-settled ModelStepOutcome as the single terminal authority.

We are retracting that recommendation. It was wrong, and the reason came out of reviewing #3549.

Two facts about the installed SDK at this head:

• @ai-sdk/provider-utils discards the [DONE] sentinel in its SSE transform (if (data === "[DONE]") { return; }), so nothing downstream ever observes it.
• @ai-sdk/openai-compatible synthesizes a finish in flush(), which runs on every ordinary response-body EOF.

So the finish this code sees is already partly synthetic: a genuine provider termination and a bare socket EOF arrive in the same shape. ModelStepOutcome is a derived settlement over that, and other work in flight (#3549) proposes to rewrite it further — that PR would turn a clean EOF before [DONE] into a normal stop, and a truncated generation's tool-calls outcome into an authorized one.

If this guard consumed the settled outcome as its only terminal authority, that rewrite would flow straight through it and authorize exactly the truncated tool call this PR exists to block. A safety gate must not derive its evidence from a settlement another layer may rewrite. Keeping independent, unrewritten evidence here is the correct design, not incidental duplication.

What still stands from that review:

• The [P1] on positive proofs is unaffected: ToolCallSafetyDecision.action declares execute | retry | reject, retry has no producer or consumer anywhere, and under raw evidence a rejected decision and an absent proof reach the same fail-closed result. Representing only positive proofs still removes an impossible state.
• The [P2] comment-density and fixture-reuse items and the [P3] shared incomplete-finish predicate are unchanged.

On the original maintainer question — why is a small safety boundary this large — the answer is now firmer than before: part of what looked like redundancy is load-bearing. The guard's own terminal evidence is doing work that the settled outcome cannot be trusted to do. What is genuinely deletable is the impossible retry state and the duplicated prose, not the independent evidence path.

Apologies for the churn; better to correct it here than to have you delete a boundary on our advice.

Reviewed exact head c14b3b907c8065ce1134324b6425eb9d4b99c45a against base efddab2fbb15c5e34472cb88770605ee77bb9d24 and current-main merge-tree. NO-GO: one P1 and one P2 remain, inline.

• [P1][①] Schema transforms/defaults are lost at execution. The guard correctly makes raw bytes the proof authority, but its bare JSON.parse value replaces the AI SDK’s schema-projected value. ToolRuntime validates and discards the parsed result, so real defaults/preprocessing do not reach implementations. An exact production-path probe reproduced a missing Zod default. The fix is to schema-parse the raw-proved value and execute that returned value, without trusting a divergent SDK projection.
• [P2][①] A normal Google mixed-delivery response rejects a legitimate zero-argument sibling. The installed Google adapter emits start/end/final-call with no delta for a no-arg call while argument-bearing siblings emit deltas. The request-global hadRawArgumentEvidence therefore blocks the no-arg call. Track the per-id atomic lifecycle (matching start/end, zero deltas, matching name) so id substitution still fails closed.

Three stale findings are closed with evidence in their threads: the external dependency is removed, the PR description now matches the Maka-owned implementation, and the installed Google adapter independently establishes the atomic event shape that the old citation did not. The positive-proof simplification is also complete.

Verification on this head: focused guard + production-path suites 23/23; ai-sdk-backend 189/189; diff check and merge-tree against main@183fe9fb32b794878bf8dfa27e6d9ff5eed7dbb4 are clean. Hosted test is completed/success on this exact SHA. GitHub still shows APPROVED only because the approval is bound to stale head 80dacb4b; it is not evidence for this head.

AI-assisted review; the reviewer is accountable for the reproduced control flow and exact-head evidence.

Reviewed at 3ac4b1c1b7e2755d65a7c8962c750ca2b3ca72f3. No P0-P3 from this pass. One gate blocker, which is not yours to fix in code.

The red test job is inherited from the base, not caused by this PR

src/__tests__/codex-session-adapter.test.ts(267,26):
  error TS2304: Cannot find name 'decodeStoredMessage'.

Three steps establish the attribution:

1. This PR does not touch that file — it is absent from git diff --name-only merge-base..head.
2. At merge-base 4852d492 the file is self-consistent: line 26 imports decodeStoredMessage, line 193 uses it, and there is no usage at line 267 at all.
3. Current main is already repaired: the import is decodeCanonicalMessage, and both call sites (194 and 267) were updated. decodeStoredMessage no longer appears in that file on main.

A pull_request run builds the merge of base and head, so this failure comes from the base as it stood when the run was created, not from this branch. That also means re-running the workflow does not help — attempt 3 reproduced the identical error, because a re-run replays the same merge ref rather than recomputing it against current main. This needs a rebase or a merge of current main, i.e. a new push. No code change is required from you.

Not graded, but also not treated as green.

The three questions this review was scoped to

Proportion first, because it determines how to read the rest. The 2156 added lines are not one condition change. Roughly 1507 are tests — length-cutoff-tool-execution-repro.test.ts (+988, the reproduction), tool-call-execution-guard.test.ts (+460), and +59 elsewhere. The ~663 lines of production code are the new tool-call-execution-guard.ts (+397), ai-sdk-backend.ts (+139), model-protocol.ts (+65), tool-runtime.ts (+36) and model-adapter.ts (+26). This adds a per-call execution-proof subsystem, not a predicate — a proportionate size for a new safety invariant shipped with its reproduction.

Is the new gate strictly stronger, or merely different? Strictly stronger — a narrowing, not a crossing. isTerminalSafe (:310) admits only terminal.kind === 'finish' with reason stop or tool-calls; undefined, other and unknown are treated as ambiguous and deferred to providerReason; everything else, length included, is refused. The class newly refused is exactly length truncation, which is the bug being fixed — the reproduction file says so in its name. On top of the terminal state, the gate also requires positive per-call evidence (proofs for raw bytes, atomicProofs for atomic delivery, mutually exclusive by construction). So the gate is "terminal-safe AND this call carries positive proof".

What happens when the stream does not end normally? It fails closed: the tool does not execute. An interrupt, timeout or error leaves terminal either pending or finish with an unsafe reason, and isTerminalSafe is false. Of the two possible defects in opposite directions, this picks "do not execute", which is the right choice for tools with side effects — running one on truncated arguments is far worse than not running it. The observeRawChunk state machine is careful in the same way: tool evidence arriving after the terminal has settled triggers blockTerminal to prevent a race, and a duplicate start, a delta without a start, a duplicate end, or a toolName that changes mid-stream are all marked invalid.

One thing this review nearly got wrong, recorded because it affects how you should read it

The tool-runtime.ts change — validateDeclaredToolArgs now returning the schema's parsed value, with executionArgs reassigned to it — initially looked like a second, unrelated concern riding along in this PR. Reading the existing review threads corrected that: it is the required fix for a P1 that this PR's own provedValue introduction exposed. Scope creep and a fix made necessary by the change under review are indistinguishable in a diff; only the history separates them.

Thread state

Two threads were resolved as part of this pass: the note on this file's header comment (praise, never a finding) and the P2/P1 on mixed delivery, which the author fixed by having validateDeclaredToolArgs return the parsed product that the downstream consumers — the permission check, the persisted tool_start and tool_call arguments, the loop-gate signature, and tool.impl — all read.

One thread remains unresolved, deliberately: the question on ai-sdk-backend.ts:2743 about switching the streamed call's input from toolCall.input to the proved value. It is labelled as a question rather than a finding and was never graded, because it was not reproduced. It is not a blocker. It is left open because resolving it would amount to answering on the author's behalf.

Re-review at 3ac4b1c1. No remaining P0–P3 — both earlier findings are fixed at the right authority boundary.

• The raw proof still reaches ai-sdk-backend, but ToolRuntime now takes the declared schema's parsed value as the single source of execution args for permission, persistence and impl. A regression that feeds raw {path:'safe.md'}, a divergent SDK payload {path:'evil.md', content:'untrusted'} and a schema default lands {path:'safe.md', content:'placeholder'} at the impl; a missing required field still executes zero times. The SDK input is not reinstated.
• The mixed Google shape now requires per-id start/end, zero delta, name and a safe terminal; a no-arg sibling gets a canonical {}, while call-id/name substitution, a missing end and an unsafe terminal all still fail closed.

Not approving yet, on gates rather than findings — two things:

1. A stale test fixture (not in this diff, so noting it here rather than inline). packages/runtime/src/__tests__/deferred-guard.test.ts:93 declares parameters: z.object({}), which strips unknown keys, and the keeps WriteStdin args exact … test at :139 then passes { ref, input, size } expecting them verbatim. That held while execution args came from the raw payload; now they come from the declared schema's parsed output, so the fixture parses to {} and the test fails. The fixture's premise expired — the real WriteStdin carries a strict schema with those fields. Giving the fixture the real fields or a passthrough schema should clear it.

2. No current-base CI evidence on this head. The last hosted test failed on an old merge ref for an unrelated Storage import that main has since fixed, and re-running the same merge SHA replays that ref rather than recomputing against the repaired base. A push or sync that triggers a current-base run is what this needs.

Thanks — the raw-stream completion criterion is the right place for this gate, and the 987-line repro makes the failure it prevents concrete.

Correcting my earlier note on this PR: I flagged "what do other backends do when they supply no raw evidence" as an open risk. That risk does not exist — packages/runtime/src/ai-sdk-backend.ts is the only model backend in the tree (test-only/fake-backend.ts aside), so there is no unguarded sibling path. The gate lives where the tool calls are dispatched, and that is currently the only place they are dispatched from. Worth keeping in mind only if a second backend is ever added: the guard is enforced inside the backend rather than at ToolRuntime, so a new backend would silently opt out of it rather than fail closed.

One thing that is easy to miss in a diff this size: this PR also changes what arguments reach tool.impl. validateDeclaredToolArgs now returns the schema's parsed output instead of void, and executeTool reassigns executionArgs to it (tool-runtime.ts). For any tool whose schema uses z.default() or .transform(), the values passed to the implementation — and persisted into tool_start/tool_call, and used for the permission check and loop-gate signature — change from the pre-validation input to the post-validation output.

That is a defensible fix on its own, and the comment explaining it is clear. But it is a behavioural change to every tool with a defaulting or transforming schema, and the PR title only describes the raw-stream gate. It would be worth calling out in the description so it gets the attention it deserves rather than arriving as a side effect.

I did verify the repro independently, and it holds up. Built at 87c38d3 it is 31/31 green. Neutering the guard's terminal-safety check (isTerminalSafe, plus isSafeToolExecutionStepOutcome) turns 5 of those 31 red:

incremental + length: executes 0 time(s)
atomic + length: executes 0 time(s)
concurrent incremental requests sharing call_1 stay isolated
concurrent atomic requests sharing call_1 stay isolated
a zero-delta sibling is withheld when the physical request terminates unsafely

Restoring the file returns it to 31/31. So the suite is genuinely load-bearing — it fails for the reason it claims to, not incidentally.